Skip to content

Comments

Basic implementation of PKCE#1652

Closed
Teeed wants to merge 1 commit intodexidp:masterfrom
Teeed:PKCE_upstream
Closed

Basic implementation of PKCE#1652
Teeed wants to merge 1 commit intodexidp:masterfrom
Teeed:PKCE_upstream

Conversation

@Teeed
Copy link
Contributor

@Teeed Teeed commented Feb 14, 2020

This is my own implementation of PKCE, it is similar to #1407 but this version could be directly merged.

Issue #1114

@Teeed Teeed requested a review from sagikazarmark February 17, 2020 09:57
@mfmarche
Copy link

mfmarche commented Feb 24, 2020

Hi @Teeed , I tried your PR and it was mostly functional.

It seems there is no way to get to the handleAuthCode without actually passing the basic auth (providing a valid client secret). Shouldn't the handling of the secret be deferred only in the case that PKCE flow is not used (ie. when there is no code_verifier?). Making this change allows for PKCE to commence without a secret.

@justin-slowik
Copy link
Contributor

justin-slowik commented May 11, 2020

Has there been any progress on implementing PKCE? I've been working on a branch which implements the OAuth 2 Device Authorization Grant, and I know that to make a fully featured native app flow, PKCE should be included as well. Would be beneficial not to duplicate work

deric
deric reviewed Jun 16, 2020
View reviewed changes
server/handlers.go
}

if codeChallengeFromStorage != calculatedCodeChallenge {
s.tokenErrHelper(w, errInvalidRequest, "invalid code_verifier.", http.StatusBadRequest)
Copy link
Contributor

@deric deric Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to RFC 7636, errInvalidGrant should be returned.

code_verifier == code_challenge.
If the values are equal, the token endpoint MUST continue processing
as normal (as defined by OAuth 2.0 [RFC6749]). If the values are not equal, an error response indicating "invalid_grant" as described in Section 5.2 of [RFC6749] MUST be returned.

@Teeed
Copy link
Contributor Author

Teeed commented Jun 16, 2020 •
edited
Loading

Yeah. First of all: sorry for not responding for such a long time. Thanks for raising issues! 👍

Company which I work for (and which allowed me to push these changes to upstream) decided that this changes are no longer needed and hence development of these changes was stalled. I currently work on different projects, so I do not have time (and business reason 😕) to fix raised issues.

If anyone wants to continue this work - please do!

@heidemn-faro
Copy link
Contributor

heidemn-faro commented Jul 20, 2020 •
edited
Loading

Hi @Teeed, thanks for your efforts here.
Please allow me a stupid question, since I'm not familiar with the codebase:

Does this add:

  1. PKCE for the client (e.g. a single-page app) connecting to Dex, or
  2. PKCE for Dex connecting to the upstream IdP (if this makes sense at all...)?

We would be interested in case 1.

@heidemn-faro
Copy link
Contributor

heidemn-faro commented Jul 31, 2020

Hi @Teeed ,

If anyone wants to continue this work - please do!

Our company wants to finish your work.
Could you please sign off your commit, meaning that you accept the Dex DCO?
Otherwise, I'm not sure if we're allowed to reuse your work in our follow-up PR.

Signing off the commit should work like this:

git checkout <BRANCH>
git commit --amend --signoff
git push --force-with-lease <ORIGIN> <BRANCH>

Thanks

@jarrettprosser jarrettprosser mentioned this pull request Aug 12, 2020
Closed
@Teeed
Basic implementation of PKCE
2879801 
Signed-off-by: Tadeusz Magura-Witkowski <tadeuszmw@gmail.com>
@oponder oponder mentioned this pull request Aug 25, 2020
Merged
@HEllRZA HEllRZA mentioned this pull request Aug 26, 2020
Merged
@HEllRZA
Copy link
Contributor

HEllRZA commented Oct 26, 2020

This PR can be closed as #1784, which is based on this, has been merged. Thanks again to @Teeed for his work!

@Teeed
Copy link
Contributor Author

Teeed commented Oct 26, 2020

Closing, as these changes got merged with #1784

@Teeed Teeed closed this Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sagikazarmark sagikazarmark Awaiting requested review from sagikazarmark

1 more reviewer

@deric deric deric left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Teeed @mfmarche @justin-slowik @heidemn-faro @HEllRZA @deric